Issue 52339: Try to use shorter URIs for domains when possible#6805
Issue 52339: Try to use shorter URIs for domains when possible#6805labkey-susanh merged 7 commits intodevelopfrom
Conversation
labkey-susanh
changed the title
labkey-jeckels
left a comment
labkey-jeckels
left a comment
There was a problem hiding this comment.
One comment but looks promising.
| return INSTANCE; | ||
| } | ||
|
|
||
| public static DbSequence getLsidPrefixDbSeq(Container container, String lsidPrefix, int batchSize) |
There was a problem hiding this comment.
Presumably this is safe because container RowIds are included in many LSIDs, but since containers can be moved, would it make sense to have a single sequence used server-wide?
There was a problem hiding this comment.
I can certainly change the usage for Lists so it uses the root container. If this method is updated now, we'd need to change the usage from the Experiment module so it can skip over LSIDs already in use from the project-scoped LSIDs.
@XingY Was there a reason not to make this server-wide when you were adding these sequences in ExperimentServiceImpl?
There was a problem hiding this comment.
Historically we had always included folder / container as part of lsid. The change I introduced was switching from data type name to use a db sequence when generating lsid, in order to support datatype/data renaming. LSIDs are not updated during a folder move, and shouldn't cause issues/conflict.
There was a problem hiding this comment.
It isn't a move that would cause the conflict but the restarting of the sequence if we switch it to use a different container where the conflict may occur. (Previous project's sequence value of 1 vs. new project's sequence value of 1.) But, yes, when there is a folder id in the LSID, there won't be a problem here.
There was a problem hiding this comment.
My future self may not thank me for this, but I'm opting for leaving the Experiment usages as is and adding a method that provides for an easy way to use a root container sequence, which we'll use for lists.
| public static Lsid generateDomainURI(String name, Container c, KeyType keyType, @Nullable ListDefinition.Category category) | ||
| public static Lsid generateDomainURI(Container c, KeyType keyType, @Nullable ListDefinition.Category category) | ||
| { | ||
| String type = getType(keyType, category); |
There was a problem hiding this comment.
Not related to this PR, but surprised to see category is hard wired to picklist.
if (category != null) type = PicklistDomainKind.NAMESPACE_PREFIX;
| { | ||
| String suffix = "Folder-" + container.getRowId(); | ||
| return (new Lsid(DOMAIN_DEFAULT_VALUE_LSID_PREFIX, suffix, domain.getName())).toString(); | ||
| DomainKind<?> kind = domain.getDomainKind(); |
There was a problem hiding this comment.
Would this cause backward compatibility issue? If a default value is set prior to this change, would it be lost since the default lsid is different now?
There was a problem hiding this comment.
I think this will be ok because when the domain is user-defined, the objectId from the domain LSID is the same as the encoded name. I tested it locally, but it would be good to double check by creating a list while on develop that has some default values defined for properties then verifying the default values still show up when using the list on this branch.
3 participants
Rationale
Issue 52339
This doesn't address the creation of long domain URIs that might exist out there, but paves the way toward shorter URIs in other scenarios.
Related Pull Requests
Changes
getLsidPrefixDbSeqtoLsidManagerfor better accessibilitygetLsidPrefixDbSeqfor creating domain URIs for listsDefaultValueServiceImplto try to use these shorter URIs as appropriate as well.